-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add local module support to the cloudformation package command #9124
base: develop
Are you sure you want to change the base?
Conversation
Includes a basic test case for unit testing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9124 +/- ##
==========================================
- Coverage 0.08% 0.07% -0.01%
==========================================
Files 210 213 +3
Lines 16955 17535 +580
==========================================
Hits 14 14
- Misses 16941 17521 +580 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid start. I have some corrections and some questions.
overrides = resource_overrides[attr_name] | ||
resource[attr_name] = merge_props(original, overrides) | ||
|
||
def resolve(self, k, v, d, n): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there are better flows here. You pass in k,v but also the dict and parent k that holds k, v? From what I can tell the big reason for that is to switch an object using an intrinsic function back into a value. You are taking advantage of the fact that dict/list are sent by reference. It feels like an approach that does a return instead of relying on a reference would read better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean something like d[n] = self.resolve(k, v)
? Maybe that is better. But I am hesitant to change it, this was the trickiest piece of code to get right.
|
||
!GetAtt Foo.Bar becomes !GetAtt ModuleNameFoo.Bar | ||
""" | ||
if not isinstance(v, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a string since GetAtt supports both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean treating GetAtt like a Ref? Just !GetAtt LogicalId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, if I'm reading this correct if GetAtt is !GetAtt Foo.Bar
it wouldn't be a list and we would raise an error unless the yaml parser is converting it to a list. If that's the case its also worth noting Fn::GetAtt: "Foo.Bar"
is also valid.
this is a valid template:
Resources:
Bucket:
Type: AWS::S3::Bucket
Outputs:
BucketArn:
Value:
Fn::GetAtt: "Bucket.Arn"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth noting I forgot to look what yaml parser we are using here so maybe this is covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws-cli/awscli/customizations/cloudformation/yamlhelper.py
Lines 36 to 40 in 804662b
if tag == "GetAtt" and isinstance(node.value, str): | |
# ShortHand notation for !GetAtt accepts Resource.Attribute format | |
# while the standard notation is to use an array | |
# [Resource, Attribute]. Convert shorthand to standard format | |
value = node.value.split(".", 1) |
!GetAtt
into a list this will miss the conversion of Fn::GetAtt: "foo.bar"
and those will remain as strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the quotes were stripped off already?
I was thinking about how to refer to values from a template and noticed that that is a place where the leaky abstraction might be a downside (most of it is true for the overrides too, but it feels more expected there): If you want to reference a resource in a module you would use e.g.
Having an "outputs" or "outputs" equivalent could solve this. e.g Parent template Modules:
Content:
Source: ./basic-module.yaml
Properties:
Name: foo
Outputs:
ExampleOutput:
Value: !GetAtt Content.BucketArn basic-module.yaml Parameters:
Name:
Type: String
Resources:
Bucket:
Type: AWS::S3::Bucket
Properties:
BucketName: !Ref Name
Outputs:
BucketArn: !GetAtt Bucket.Arn Output from the package command Resources:
ContentBucket:
Type: AWS::S3::Bucket
Properties:
BucketName: foo
Outputs:
ExampleOutput:
Value: !GetAtt ContentBucket.Arn |
Agreed with Ben on this one. You could still allow for an override capability that allows for a missing output that may be needed but having to understand the module resource names can get frustrating and prevents flexible in changing them. There are going to be limitations in this approach that are similar to modules. Adding mappings/conditions into a module will limit its usage because it cannot rely on another resource from the parent template or other module. |
I don't see any reason not to add Outputs. It's definitely a more predictable way to reference resources in the module. I still want the Overrides section and the ability to reference things in the module directly if you know the name, but that can be considered a more advanced use case. If you're consuming a 3rd party module, the risk is higher than if you're referencing your own local modules. |
Implemented here: 160be7c |
This PR adds the ability to write local CloudFormation modules, which are YAML files that contain Parameters and Resources. These modules can be included in a parent template in a new
Modules
section. Modules are configured by a combination of formalParameters
(which coincide with theProperties
defined in the parent) and anOverrides
attribute, which allows users full control over the rendered output. This is a port from the functionality that exists in the Rain CLI. See the snippets below for a basic example.Parent template
basic-module.yaml
Output from the package command
This PR is a proof of concept with a limited set of functionality.
Design Decisions
Modules can be imported into a template (or into a module itself) from a new
Modules
section. In Rain and in registry modules, this section does not exist. Instead, the imported content is configured in a resource and the source of the module is embedded in theType
attribute. This PR implements both ways of configuring an imported module, giving customers the option of whichever feels more intuitive. The benefit of using aModules
section is that it will be more intuitive if we decide to allow injecting other sections other thanResources
, likeOutputs
.Logical IDs are a concatenation of the module name and the logical id defined in the module. Unlike registry modules, users do not have the option of referring to the resources with a dot between the names.
The
Overrides
section is not considered an escape hatch of last resort. It is an encouraged way to configure modules without requiring the module author to anticipate every possible need. IaC abstractions are leaky by nature and we expect users to understand the internals of any module they import. Deciding how many actualParameters
to include in a module is highly opinionated and left up to authors. One caveat here is thatOverrides
are fragile, since they will break if the module author changes any of the logical IDs in the module. See item 5,Outputs
, as a way to mitigate this.This functionality has been added to the
aws cloudformation package
command. We could have created a new dedicated command for module packaging, likesynth
. This is an easily reversible decision. The code is ported from the Rainpkg
command, which was originally written to mimic the behavior ofaws cloudformation package
, so it seems like the right place to put it. One annoyance is that if you are only doing local packaging, you still have to supply thes3-bucket
argument, and you need to be connected to an AWS account.Modules can define
Outputs
, which are key-value pairs that can be referenced by the parent. This gives module authors a formal way to pass values to the consumer without them needing to rely onOverrides
, which will be fragile if the module author changes any of the names in the module.Instead of using
Fn::ForEach
as a way of implementing loops, we added aMap
attribute to a local module resource. The module output is repeated for each element in the map, substituting$MapValue
and$MapIndex
in module Properties. This can only be used with lists that can be resolved at build time.What's not done in this PR
We need to decide what's necessary for an initial version and what can wait
[ ] - Many of the related Rain features like Metadata commands and Constants
[ ] - Conditionals in modules, to emit or omit resources based on Parameters
How to test this PR
Check out my branch and install the aws cli into a local Python environment.